LSP Notification Message Type #749
Conversation
dyl10s
commented
Apr 4, 2025
dyl10s
commented
- Added lsp notification message type, as the todo comment stated the difference is if an ID is provided or not.
- A few of the messages that were being handled were notifications, so they are all now parsed/typed correctly.
Co-authored-by: Copilot <[email protected]>
internal/lsp/lsproto/jsonrpc.go
Outdated
| var raw struct { | ||
| JSONRPC JSONRPCVersion `json:"jsonrpc"` | ||
| ID *ID `json:"id"` | ||
| ID *ID `json:"id,omitempty"` |
There was a problem hiding this comment.
Generally speaking, we should be using omitzero (new in Go 1.24) rather than omitempty.
There was a problem hiding this comment.
There are other of these, FWIW, I just commented on the one.
There was a problem hiding this comment.
I went ahead and normalized this file to always use omitzero!
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/lsp/server.go:193
- [nitpick] For unknown request methods that include an ID, consider returning an error (such as using sendError) rather than solely logging the event, to notify clients of invalid requests.
s.Log("unknown method", req.Method)
|
I am currently redoing this code to deal with the fact that we need to handle requests, notifications, and responses going both directions over the pipe. For this, I don't actually think we should split these types apart, but rather we'll have one thing that's just "Message" that represents whatever can be sent over the wire. With that in mind, I think I'm going to close this; sorry I was slow to review this but I don't think it will line up with what we're going to need right away. |
